Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

document + UI test E0208 and make its output more user-friendly #106931

Merged
merged 1 commit into from
Jan 19, 2023
Merged

document + UI test E0208 and make its output more user-friendly #106931

merged 1 commit into from
Jan 19, 2023

Conversation

Ezrashaw
Copy link
Contributor

@Ezrashaw Ezrashaw commented Jan 16, 2023 •
edited by GuillaumeGomez
Loading

Cleans up E0208's output a lot. It could actually be useful for someone learning about variance now. I also added a UI test for it in tests/ui/error-codes/ and wrote some docs for it.

r? @GuillaumeGomez another error code, can't be bothered to find the issue :P. Obviously there's some compiler stuff, so you'll have to hand it off.

Part of #61137.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 16, 2023
@GuillaumeGomez
Copy link
Member

Looks good to me, thanks! As you mentioned, it needs approval from the compiler team too.

r? @estebank

@rustbot rustbot assigned estebank and unassigned GuillaumeGomez Jan 16, 2023
cjgillot
cjgillot reviewed Jan 16, 2023
View reviewed changes
tests/ui/variance/variance-associated-consts.rs Outdated
@@ -10,7 +10,7 @@ trait Trait {
}

#[rustc_variance]
struct Foo<T: Trait> { //~ ERROR [o]
struct Foo<T: Trait> { //~ ERROR E0208
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is first a test tool for the compiler.
In this idea, losing the short version of the information makes the test much less useful.

compiler-errors
compiler-errors requested changes Jan 16, 2023
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with cjgillot -- these UI tests are specifically checking for [+, -], etc error outputs to assert the variances of an item's generic parameters.

Replacing them with E0208 and a generic error message makes all of them far less useful -- it seems very easy to accidentally --bless a test and not notice the output change, versus // ERROR attributes which need to be "blessed" manually.

Secondly, I don't think a structured suggestion to remove the attribute is necessarily useful.

@Ezrashaw
Copy link
Contributor Author

Ezrashaw commented Jan 16, 2023 •
edited
Loading

@compiler-errors Yeah, I get that and perhaps we can work the old behavior back in somehow. The point though, is that if an error has a code, then it should be documented, tested and usable from a users perspective. This could become helpful for someone learning about variance, etc. Perhaps we can add a note with just the old one like //|~ NOTE [..], do you have to then mark all subdiagnostics?

EDIT: Alternatively, if you think this really should be purely internal then it shouldn't have an error code.

@compiler-errors
Copy link
Member

This error is purely internal. It certainly isn't a good candidate for stabilization in the state that it's currently written, not even considering that it's a rustc_ attr and can't even be used by non-nightly users. Perhaps it doesn't need an error code.

--

The problem with //~| NOTE attrs is that we want to point to each variant, so we'd need to do something like:

fn foo<
  T, // NOTE +
  S, // NOTE o
>() {}

Or something... seems like a lot of overhead.

@Ezrashaw
Copy link
Contributor Author

@compiler-errors Oh, I am absolutely not saying it should be stabilized, just that perhaps it could be used for more than just testing. What about having #[rustc_variance] and #[rustc_variance(terse)] or something like that?

@estebank
Copy link
Contributor

r? @compiler-errors

@rustbot rustbot assigned compiler-errors and unassigned estebank Jan 17, 2023
@compiler-errors
Copy link
Member

The point though, is that if an error has a code, then it should be documented, tested and usable from a users perspective.

Well in that case, I'd rather just remove the error code from this error, and keep it fully internal.

If we want to add a new error code and some other variance inspection tool (maybe via a warning rather than an error), then it should be suggested and implemented separately.

@compiler-errors compiler-errors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 18, 2023
@Ezrashaw
Copy link
Contributor Author

@compiler-errors I've removed the error code from #[rustc_variance]. The error code docs have been updated to reflect that it is no longer emitted but the documentation itself remains. I assume @GuillaumeGomez is ok with this.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 18, 2023
@compiler-errors
Copy link
Member

This is fine then

@bors r+

@bors
Copy link
Contributor

bors commented Jan 18, 2023

📌 Commit 708861e has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 18, 2023
@GuillaumeGomez
Copy link
Member

Yup I'm fine with it. Sorry didn't see the notification.

@GuillaumeGomez GuillaumeGomez mentioned this pull request Jan 19, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 19, 2023
@bors
Auto merge of rust-lang#107064 - GuillaumeGomez:rollup-pbgu6r3, r=Gui…
79335f1 
…llaumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#105977 (Transform async `ResumeTy` in generator transform)
 - rust-lang#106927 (make `CastError::NeedsDeref` create a `MachineApplicable` suggestion)
 - rust-lang#106931 (document + UI test `E0208` and make its output more user-friendly)
 - rust-lang#107027 (Remove extra removal from test path)
 - rust-lang#107037 (Fix Dominators::rank_partial_cmp to match documentation)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 246daa4 into rust-lang:master Jan 19, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 19, 2023
@Ezrashaw Ezrashaw deleted the docs-e0208 branch May 13, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot left review comments

@compiler-errors compiler-errors compiler-errors requested changes

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@compiler-errors compiler-errors

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.68.0
Development

Successfully merging this pull request may close these issues.
7 participants
@Ezrashaw @GuillaumeGomez @compiler-errors @estebank @bors @cjgillot @rustbot